Add depends_on :linux and :macos #3270

sjackman · 2017-10-05T16:40:22Z

Have you followed the guidelines in our Contributing document?
Have you checked to ensure there aren't other open Pull Requests for the same change?
Have you added an explanation of what your changes do and why you'd like us to include them?
Have you written new tests for your changes? Here's an example.
Have you successfully run brew tests with your changes locally?

Allow a formula to declare whether it depends on macOS or Linux.

MikeMcQuaid · 2017-10-06T03:31:47Z

Library/Homebrew/dependency_collector.rb

@@ -108,7 +108,8 @@ def parse_symbol_spec(spec, tags)
    case spec
    when :x11        then X11Requirement.new(spec.to_s, tags)
    when :xcode      then XcodeRequirement.new(tags)
-    when :macos      then MinimumMacOSRequirement.new(tags)
+    when :linux      then LinuxRequirement.new
+    when :macos      then tags.empty? ? MacOSRequirement.new : MinimumMacOSRequirement.new(tags)


make MacOSRequirement.new handle the tags.empty? case instead of having the meaning overloaded.

ilovezfs · 2017-10-06T04:27:07Z

How do you specify both that macOS is required and that a minimum version is required? My understanding is depends_on :macos => :el_capitan is a no-op on Linux.

sjackman · 2017-10-06T05:47:43Z

To specify that both that macOS is required and that a minimum version is required:

depends_on :macos
depends_on MinimumMacOSRequirement => :el_capitan

ilovezfs · 2017-10-06T05:49:04Z

That seems like pretty yucky syntax to me.

MikeMcQuaid · 2017-10-06T14:11:55Z

Library/Homebrew/requirements/minimum_macos_requirement.rb

    super
  end

-  satisfy(build_env: false) { MacOS.version >= @version }
+  satisfy(build_env: false) do
+    if @version


next false if !OS.mac? || !@version

May be worth taking the same approach below, too.

Done. Thanks for the suggestion, Mike. The logic needed tweaking. When OS.mac? is true and @version is nil, the result should be true.

MikeMcQuaid · 2017-10-06T14:12:10Z

Library/Homebrew/requirements/minimum_macos_requirement.rb

@@ -3,18 +3,32 @@
 class MinimumMacOSRequirement < Requirement


Might be worth renaming this now to be consistent, what do you think?

The name of the class is a component of the DSL. The name MinimumMacOSRequirement is used by two formulae in core:

nordugrid-arc.rb: depends_on MinimumMacOSRequirement => :yosemite phantomjs.rb: depends_on MinimumMacOSRequirement => :lion

We can add it to compat and update them to use depends_on :macos

Got it. Would you like to suggest a name?

MacOSRequirement?

I've renamed MinimumMacOSRequirement to MacOSRequirement and added MinimumMacOSRequirement to compat.

MikeMcQuaid · 2017-10-08T00:16:46Z

Library/Homebrew/compat/requirements/minimum_macos_requirement.rb

@@ -0,0 +1,3 @@
+require "requirements/macos_requirement"


Stick this file's contents in homebrew/Library/Homebrew/compat/requirements.rb

MikeMcQuaid · 2017-10-08T00:17:23Z

Library/Homebrew/requirements/macos_requirement.rb

+  fatal true
+
+  def initialize(tags = [])
+    @version = MacOS::Version.from_symbol tags.first unless tags.empty?


MacOS::Version.from_symbol(tags.first) just to aid visual parsing.

MikeMcQuaid · 2017-10-08T00:18:11Z

Library/Homebrew/requirements/macos_requirement.rb

+
+  satisfy(build_env: false) do
+    next false if !OS.mac? && !@version
+    next true if !OS.mac? || !@version


This is a little hard to parse above. Can you maybe make it if/elses? This may contradict what I said earlier: sorry!

Factoring out the minimum_version? utility method also helped make this logic more clear.

MikeMcQuaid · 2017-10-08T00:18:32Z

Library/Homebrew/requirements/macos_requirement.rb

+  end
+
+  def message
+    return "macOS is required." if !OS.mac? || !@version


if !OS.mac? || !@version could be a utility method, I think, it's repeated 3 times.

MikeMcQuaid · 2017-10-08T20:41:00Z

Library/Homebrew/requirements/macos_requirement.rb

+    super
+  end
+
+  def minimum_version?


minimum_version_provided or passed or specified something would be nicer to make it clear that this doesn't mean the minimum version is met.

MikeMcQuaid · 2017-10-08T20:41:59Z

Tests failing on Linux (shouldn't assume MacOS is specified there): https://travis-ci.org/Homebrew/brew/jobs/285144949

sjackman · 2017-10-09T16:21:40Z

Fixed up.

MikeMcQuaid · 2017-10-09T22:11:53Z

@sjackman Still got 🔴.

sjackman · 2017-10-10T05:02:16Z

😬

sjackman · 2017-10-14T18:30:00Z

Good to merge?

MikeMcQuaid · 2017-10-17T20:34:06Z

Library/Homebrew/requirements/macos_requirement.rb

+  end
+
+  satisfy(build_env: false) do
+    next OS.mac? || @version unless minimum_version_specified?


Sorry, last nite: can you split this into two next checks and return a boolean value?

Move MinimumMacOSRequirement to compat.

MikeMcQuaid · 2017-10-18T08:43:53Z

Thanks @sjackman and sorry for the nit picking!

sjackman · 2017-10-18T15:13:10Z

No worries. Thanks for merging, Mike.

MikeMcQuaid requested changes Oct 6, 2017

View reviewed changes

sjackman force-pushed the os-requirement branch 2 times, most recently from c82d73b to eb3ba7c Compare October 6, 2017 06:10

MikeMcQuaid requested changes Oct 6, 2017

View reviewed changes

sjackman force-pushed the os-requirement branch from eb3ba7c to fa05a65 Compare October 6, 2017 17:01

MikeMcQuaid requested changes Oct 8, 2017

View reviewed changes

sjackman force-pushed the os-requirement branch from 67dd0a3 to 9cccda7 Compare October 8, 2017 17:24

MikeMcQuaid reviewed Oct 8, 2017

View reviewed changes

sjackman force-pushed the os-requirement branch from 9cccda7 to 88e0501 Compare October 9, 2017 16:20

sjackman force-pushed the os-requirement branch from 88e0501 to 507f248 Compare October 10, 2017 05:01

sjackman mentioned this pull request Oct 16, 2017

Cask: require macOS #3325

Closed

5 tasks

MikeMcQuaid requested changes Oct 17, 2017

View reviewed changes

sjackman added 2 commits October 17, 2017 16:16

Add depends_on :linux and :macos

95e144a

Rename MinimumMacOSRequirement to MacOSRequirement

230c87a

Move MinimumMacOSRequirement to compat.

sjackman force-pushed the os-requirement branch from 507f248 to 230c87a Compare October 17, 2017 23:17

MikeMcQuaid approved these changes Oct 18, 2017

View reviewed changes

MikeMcQuaid merged commit c7d21da into Homebrew:master Oct 18, 2017

sjackman deleted the os-requirement branch October 18, 2017 15:13

Homebrew locked and limited conversation to collaborators May 4, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add depends_on :linux and :macos #3270

Add depends_on :linux and :macos #3270

sjackman commented Oct 5, 2017

MikeMcQuaid Oct 6, 2017

sjackman Oct 6, 2017

ilovezfs commented Oct 6, 2017

sjackman commented Oct 6, 2017

ilovezfs commented Oct 6, 2017

MikeMcQuaid Oct 6, 2017

MikeMcQuaid Oct 6, 2017

sjackman Oct 6, 2017

MikeMcQuaid Oct 6, 2017

sjackman Oct 6, 2017

MikeMcQuaid Oct 6, 2017

sjackman Oct 6, 2017

MikeMcQuaid Oct 6, 2017

sjackman Oct 6, 2017

MikeMcQuaid Oct 8, 2017

MikeMcQuaid Oct 8, 2017

MikeMcQuaid Oct 8, 2017

sjackman Oct 8, 2017

MikeMcQuaid Oct 8, 2017

MikeMcQuaid Oct 8, 2017

MikeMcQuaid commented Oct 8, 2017

sjackman commented Oct 9, 2017

MikeMcQuaid commented Oct 9, 2017

sjackman commented Oct 10, 2017

sjackman commented Oct 14, 2017

MikeMcQuaid Oct 17, 2017

sjackman Oct 17, 2017

MikeMcQuaid commented Oct 18, 2017

sjackman commented Oct 18, 2017

		@@ -3,18 +3,32 @@
		class MinimumMacOSRequirement < Requirement

Add depends_on :linux and :macos #3270

Add depends_on :linux and :macos #3270

Conversation

sjackman commented Oct 5, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ilovezfs commented Oct 6, 2017

sjackman commented Oct 6, 2017

ilovezfs commented Oct 6, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MikeMcQuaid commented Oct 8, 2017

sjackman commented Oct 9, 2017

MikeMcQuaid commented Oct 9, 2017

sjackman commented Oct 10, 2017

sjackman commented Oct 14, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MikeMcQuaid commented Oct 18, 2017

sjackman commented Oct 18, 2017